Skip to content

fix(core): use v8 serializer in v19 #31222

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: 19.8.x
Choose a base branch
from
Open

Conversation

AgentEnder
Copy link
Member

See #30516, this is a backport to 19.8.x

…void issues when JSON.stringify would fail

fix(core): v8 serializer should be able to hash tasks

feat(core): use v8 serializer for daemon messages by default
@AgentEnder AgentEnder requested a review from a team as a code owner May 14, 2025 22:30
@AgentEnder AgentEnder requested a review from FrozenPandaz May 14, 2025 22:30
Copy link

vercel bot commented May 14, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
nx-dev ⬜️ Ignored (Inspect) Visit Preview May 20, 2025 6:45pm

Copy link

Failed to publish a PR release of this pull request, triggered by @AgentEnder.
See the failed workflow run at: https://github.com/nrwl/nx/actions/runs/15032407607

Copy link

nx-cloud bot commented May 14, 2025

View your CI Pipeline Execution ↗ for commit 862e973.

Command Status Duration Result
nx nx-release @nx/nx-source --parallel 8 --loca... ✅ Succeeded 7m 51s View ↗
nx run-many -t build-native-wasm ✅ Succeeded 1m 8s View ↗
nx run-many --target=build-native -- --target=x... ✅ Succeeded 5m 36s View ↗
nx run-many --target=build-native -- --target=a... ✅ Succeeded 4m 11s View ↗
nx run-many --target=build-native -- --target=x... ✅ Succeeded 3m 50s View ↗
nx run-many --target=build-native -- --target=a... ✅ Succeeded 3m 29s View ↗
nx run-many --target=build-native -- --target=a... ✅ Succeeded 2m 38s View ↗

☁️ Nx Cloud last updated this comment at 2025-05-20 19:07:12 UTC

Copy link

Failed to publish a PR release of this pull request, triggered by @Cammisuli.
See the failed workflow run at: https://github.com/nrwl/nx/actions/runs/15145266778

@Cammisuli Cammisuli requested a review from a team as a code owner May 20, 2025 18:41
Copy link

🐳 We have a release for that!

This PR has a release associated with it. You can try it out using this command:

npx [email protected] my-workspace

Or just copy this version and use it in your own command:

0.0.0-pr-31222-862e973
Release details 📑
Published version 0.0.0-pr-31222-862e973
Triggered by @Cammisuli
Branch fix/v8-serialize-19-8
Commit 862e973
Workflow run 15145357862

To request a new release for this pull request, mention someone from the Nx team or the @nrwl/nx-pipelines-reviewers.

// strings
(message.startsWith('"') && message.endsWith('"')) ||
// numbers
/^[0-9]+(.?[0-9]+)?$/.test(message)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bug in the regex pattern for detecting numbers. The current pattern /^[0-9]+(.?[0-9]+)?$/ uses .? which matches any character (not just a decimal point) followed by an optional quantifier.

To properly match decimal numbers, the pattern should be /^[0-9]+(\.[0-9]+)?$/ where \. explicitly matches a literal decimal point.

Suggested change
/^[0-9]+(.?[0-9]+)?$/.test(message)
/^[0-9]+(\.[0-9]+)?$/.test(message)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +290 to +291
`No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:` +
Object.keys(pending).map((t) => ` - ${t}`)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is using Object.keys(pending) but pending is a Map, not a plain object. This will produce an empty array or incorrect keys in the error message. To properly list the pending transaction IDs, use Array.from(pending.keys()) instead:

`No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:` +
  Array.from(pending.keys()).map((t) => `  -  ${t}`)
Suggested change
`No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:` +
Object.keys(pending).map((t) => ` - ${t}`)
`No pending promise found for transaction "${tx}". This may indicate a bug in the plugin pool. Currently pending promises:` +
Array.from(pending.keys()).map((t) => ` - ${t}`)

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants